#989: allow expressions in template variable definitions - #2282
Conversation
…/989-expression-functions
Coverage Report for CI Build 33495019591Coverage increased (+0.2%) to 73.819%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions229 previously-covered lines in 9 files lost coverage.
Coverage Stats💛 - Coveralls |
Updated changelog for version 2026.08.002, including new features and bugfixes.
…ion-functions # Conflicts: # CHANGELOG.adoc
maybeec
left a comment
There was a problem hiding this comment.
Thanks for this PR, and thanks in particular for the parser itself. Locating a call with a RegEx and then scanning the argument list by hand is exactly the right call: findClosingParenthesis / parseArguments handle quoted commas, quoted parenthesis and nesting correctly, and the parametrized testForeignExpressionIsUntouched covering @media, @include, @Override and "@angular/core" is precisely the test I would have asked for. Passing unknown functions through untouched is the single most important property of this feature and you got it right and covered it. The JavaDoc on the new types is also good.
No blockers, and nothing below needs a redesign. The two I care most about are the plaintext handling of @ask-secret values and @path(..., native) not using WindowsPathSyntax.
Should-fix
@ask-secretvalues are persisted and logged in clear text -AbstractEnvironmentVariables.java:414@path(..., native)hand-rolls separator replacement instead ofWindowsPathSyntax-PathFunction.java:51isPersistent()is hard-coded totrue, so the "settings templates must not persist" requirement of #989 is not actually implemented -AbstractEnvironmentVariables.java:422, and its test only exercises a test double -ExpressionParserTest.java:198askForSecretis a verbatim copy ofaskForInput-AbstractIdeContext.java:1108DirectoryMergerExpressionTestdepends onHashtableiteration order -DirectoryMergerExpressionTest.java:34- The masking itself has no automated coverage, and the Windows git-bash path needs a manual test pass -
IdeContextConsole.java:65 - The new expression syntax is not documented anywhere -
CHANGELOG.adoc:9 - A template authoring error aborts
ide updatewith a rawIllegalArgumentException-ExpressionParser.java:89
Minor
- Secrets are
trim()ed -AbstractIdeContext.java:1121 - Function results are re-scanned by the variable resolver -
AbstractEnvironmentVariables.java:214 AbstractIdeContextTest.TEST_RESOURCESalready exists -DirectoryMergerExpressionTest.java:36- Redundant initial
matcher.find()-ExpressionParser.java:57
Scope against #989
| Requirement | Status |
|---|---|
@<function-name>([<arg>[,<arg>]*]) syntax, args always String, comma separated, trimmed, quoted with ' or " |
met |
| Args may themselves contain variables | met |
| Manual argument scanning instead of a pure RegEx | met, and better than the RegEx sketched in the issue |
@path with 1st arg path, optional 2nd arg unix (default) / native |
partial - native does not use WindowsPathSyntax (see 2) |
@ask-variable / @ask-secret, defined variable returned without asking, empty 1st arg always asks, default question text, 3rd arg default value |
met |
Values from a workspace template persisted to conf/ide.properties |
met |
| "For settings templates this should not happen since these templates are only instantiated once" | missing (see 3) |
@if-windows / @if-mac / @if-linux / @if-unix |
met |
ExpressionFunction interface + ExpressionFunctionManager registry so new releases can register more functions |
met |
Also: the issue invites a follow-up story for the maven settings.xml password prompting/encryption (@ask-maven-secret or a resolve flag telling the function it is resolving settings.xml). Worth creating it and linking it here so the plaintext-storage topic from finding 1 has an owner.
CI / DoD
All checks green, CLA signed, branch is up-to-date with main, CHANGELOG entry present under the correct milestone, PR title follows #989: .... One nit: the commit Update CHANGELOG for version 2026.08.002 does not follow the #<issue-id>: <summary> commit format (see documentation/contributing/commit.adoc).
|
Thanks for the PR. Most of my review points align with those of @maybeec mentioned earlier, so you can simply refer to my summary below; I won't be reviewing each section individually. Please feel free to contact me if you have any questions. Review: #989 — expression functions for template variables Overall: solid, well-tested implementation. The architecture matches the ticket's design (interface + manager + parser + per-OS/path/ask functions), the parser correctly handles what a regex Below are findings, most-severe first. None block merge; the first two are worth a decision. 1. Secret values flow into logs in plaintext (masking only applies to keystrokes) @ask-secret masks input while typing, but the returned value is then treated as an ordinary variable value. The integration test's own output shows it: 2. isPersistent() is hardcoded true — settings templates persist too (spec deviation) AbstractEnvironmentVariables.EnvironmentExpressionContext.isPersistent() (line 421–423) always returns true, so @ask-variable/@ask-secret persist to conf/ide.properties even when resolved from "For settings templates this should not happen since these templates are only instantiated once." The Javadoc on ExpressionContext.isPersistent() (line 41–44) even documents the intended distinction, but the implementation doesn't make it — it has no way to tell whether the current 3. A defined variable's value returned by @ask-variable is not re-resolved for nested expressions In AskFunction.apply, the defined-variable fast path returns context.getVariable(...) directly (line 71–73). That value is appended into value2 and then $[...] variables are resolved once more 4. Minor
Suggested before merge Items 1 and 2 are the only ones needing a judgment call — decide whether secrets should be masked in logs and whether the settings-vs-workspace persistence distinction is in scope now or a follow-up. Everything else is optional polish. The implementation and tests otherwise meet the Definition of Done for the core story. |
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| - | - | Generic High Entropy Secret | 3b2dd8c | cli/src/test/java/com/devonfw/tools/ide/expression/ExpressionParserTest.java | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
hohwille
left a comment
There was a problem hiding this comment.
@Paras14 thank you for your PR. This is really great work and you already included more than I was hoping for. Your implementation seems solid and functional. Awesome job 🥇
I left comments for improvement. My suggestion would be that you have a look what can be done very easy and quick and create a follow up issue for the rest so we can resolve these comments here and merge this PR which is already great as is. Thanks 🙏
| === Functions | ||
|
|
||
| * `@path` normalizes a path. | ||
| The first argument is the path. | ||
| The second argument is optional and is the literal value `unix` (the default, which replaces backslashes with slashes) or `native` (which on Windows uses `WindowsPathSyntax.WINDOWS` and therefore backslashes). | ||
| This is the remedy for values that come out with a mixed up path syntax on Windows, e.g. | ||
| `$[IDE_HOME]/software/node/node.exe` resolves to `D:\projects\my-project/software/node/node.exe` where the backslashes of the drive letter and the slashes of the rest clash. | ||
| + | ||
| [source] | ||
| ---- | ||
| @path('$[IDE_HOME]/software/node/node', unix) | ||
| @path('$[IDE_HOME]/software/node/node.exe', native) | ||
| ---- | ||
| * `@ask-variable` asks the user for the value of a variable in plain text, and `@ask-secret` does the same but reads the input with masked input. | ||
| The first argument is the name of the requested variable. | ||
| If the variable is already defined it is returned without asking. | ||
| If the empty string is given, the user is always asked and nothing is persisted. | ||
| The second argument is an optional explicit question used as the prompt. | ||
| If omitted, it defaults to `Please enter the value for the (secret) variable «NAME»:` and is required if the first argument is empty. | ||
| The third argument is an optional configuration location to persist the entered value to, analogous to the `--cfg` option: `settings`, `workspace`, `conf` or `home` (default: `conf`). | ||
| The fourth argument is an optional default value that is appended to the question in angled brackets so the user can just press return. | ||
| Provide the empty string (`''`) to allow an empty value. | ||
| If omitted or given as `null`, an empty value is not allowed and the user is asked again. | ||
| + | ||
| [source] | ||
| ---- | ||
| @ask-variable('AI_BACKEND_URL') | ||
| @ask-variable('AI_BACKEND_URL', 'Please enter the LLM backend URL:', conf, 'http://localhost:1234') | ||
| @ask-secret('AI_API_KEY', 'Please enter your API key:', conf) | ||
| ---- | ||
| * `@if-windows`, `@if-mac`, `@if-linux` and `@if-unix` insert their single argument only if the current operating system matches and otherwise resolve to the empty string. | ||
| This allows to select an OS specific value inline. | ||
| + | ||
| [source] | ||
| ---- | ||
| $[IDE_HOME]/software/mvn/bin/mvn@if-windows('.bat')@if-unix('.sh') | ||
| ---- | ||
|
|
||
| A function that is not registered by `IDEasy` is passed through entirely untouched. | ||
| This is essential since foreign configuration formats may use an `@` for their own purposes (e.g. a CSS rule `@media(max-width:600px)` or an Eclipse `@param`) and `IDEasy` must never try to resolve placeholders that are not its own. | ||
|
|
||
| === Where the values of `@ask-*` are stored and how to use the functions | ||
|
|
||
| * The value entered for an `@ask-*` call is persisted to the `ide.properties` of the chosen configuration location so that the user is asked only once. | ||
| In particular, the value entered for `@ask-secret` is _not_ encrypted; it is stored as plain text in `conf/ide.properties`. | ||
| That file is user-local and is not committed to git, and the value is masked in all log output, but the file itself is readable on the machine. | ||
| Encryption is a separate follow-up story (it also covers the case of the Maven `settings.xml`). | ||
| * Expressions belong in the workspace templates under `setup` and `update`. | ||
| Do not put an `@ask-*` call into a value of `ide.properties` itself: a value that contains an expression is resolved on _every_ variable resolution, so a `@ask-*` in `ide.properties` would prompt on every `ide` invocation (including `ide env`). | ||
| Expressions are typically used in the `update` templates to build up OS-specific or user-specific settings that would otherwise be hard to express with a plain variable, as in the following example that only resolves to a working value on the matching operating system: | ||
| + | ||
| [source] | ||
| ---- | ||
| <local-interpreter path="@path('$[IDE_HOME]/software/node/node', native)" merge:id="name()"/> | ||
| ---- | ||
|
|
There was a problem hiding this comment.
This is great but I fear that this way it will be hard to keep the documentation in sync with the code.
Maybe we can use docjar and just link to the latest JavaDoc of the functions package.
If we later follow my suggestions to express function arguments more structural with generic parsing support rather than manual handling in apply method implementation the JavaDoc itself would be fully sufficient to lookup what is needed. Otherwise we can also just put a link to the package in the GitHub source-code.
There was a problem hiding this comment.
This is valid point, let's move this to a separate issue, since linking the JavaDoc (or the GitHub source) and reworking the docs is best done on its own to keep them in sync.
@hohwille
…ion-functions # Conflicts: # CHANGELOG.adoc
…/Paras14/IDEasy into feature/989-expression-functions # Conflicts: # CHANGELOG.adoc
… @ask-secret values
…nstead of a mock context
Co-authored-by: Jörg Hohwiller <hohwille@users.noreply.github.com>
This PR fixes #989
Implements #989 and supersedes the
$[ask:...]/$[secret:...]syntax of #2179.Closes #2165.
Implemented changes
com.devonfw.tools.ide.expressionwith the expression syntax@«function-name»([«arg»[,«arg»]*])resolved during variable resolution.ExpressionFunction- interface implemented by every function.ExpressionFunctionManager- registry to look up functions by name so further functions can be registered with new IDEasy releases.ExpressionParser- locates function calls and parses their arguments. A RegEx is only used to locate the start of a call, the argument list is scanned manually since a RegEx cannot express a balanced list of arguments containing quoted commas, quoted parenthesis or nested calls.ExpressionContext- gives a function access to theIdeContext, variable lookup and persistence.@path(«path»[, unix|native])that normalises a path, by default replacing backslashes with slashes.@ask-variable(«name»[, «question»[, «default»]])and@ask-secret(...)with masked input. An already defined variable is returned without asking, an empty 1st argument always asks, and an empty string as 3rd argument permits empty input.@if-windows,@if-mac,@if-linuxand@if-unixthat insert their argument if the OS matches.IdeContext.askForSecret(String, String)analogous toaskForInput.AbstractIdeContextimplements the prompt loop and the batch mode contract and delegates reading to the new protectedreadSecretLine(), whichIdeContextConsoleoverrides withConsole.readPassword().conf/ide.propertiesso the question is only asked the first time. A value that could not be asked for in batch mode is not persisted.@path('$[IDE_HOME]/software/node')).Testing instructions
Masked input needs a real console, so run this from a normal terminal and not from the IDE.
(make sure you add your path correctly for the cli target classes: "\cli\target\classes")
IDEasy\settings\workspace\update\ai-test.properties:Make sure
MY_URL,MY_TOKENandMY_OPTIONALare not yet defined inIDEasy\conf\ide.properties.You are asked three times.
MY_URLis echoed while typing,MY_TOKENandMY_OPTIONALare not. The question forMY_TOKENis shown as given, including the parenthesis. Press enter without typing anything forMY_OPTIONAL.IDEasy\workspaces\main\ai-test.propertiescontains the three entered values andai.node.pathwith backslashes.IDEasy\conf\ide.propertiescontainsMY_URL,MY_TOKENandMY_OPTIONAL.Run again. There is no prompt and the file keeps the same values.
Remove
MY_URL,MY_TOKENandMY_OPTIONALfromIDEasy\conf\ide.propertiesand run again with--batch. There is no prompt, the workspace merge fails withCliAbortException: Aborted by end-user.and nothing is added toIDEasy\conf\ide.properties, as an undefined variable cannot be asked for in batch mode.Checklist for this PR
Make sure everything is checked before merging this PR. For further info please also see
our DoD.
mvn clean testlocally all tests pass and build is successful#«issue-id»: «brief summary»(e.g.#921: fixed setup.bat). If no issue ID exists, title only.In Progressand assigned to you or there is no issue (might happen for very small PRs)with
internal